Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new icons #2102

Merged
merged 6 commits into from
Jul 16, 2019
Merged

Add new icons #2102

merged 6 commits into from
Jul 16, 2019

Conversation

claracruz
Copy link
Contributor

@claracruz claracruz commented Jul 10, 2019

Summary

This PR adds new icons provided @ here

Name Icon
cloudDrizzle image
cloudStormy image
cloudSunny image
documents image
documentEdit image
training image
videoPlayer image

Checklist

  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • Jest tests were updated or added to match the most common scenarios

@ryankeairns
Copy link
Contributor

@claracruz we have existing/similar icons for a couple of these (document and edit) and I'm not certain the others ought to be added to the EUI icon set since they don't strike me as generally re-usable (is this for Kibana?).

My recommendation is that these icons be stored elsewhere and referenced via a path or URL as described in the 'Custom SVGs' section at the bottom of this docs page: https://elastic.github.io/eui/#/display/icons

@claracruz
Copy link
Contributor Author

@claracruz we have existing/similar icons for a couple of these (document and edit) and I'm not certain the others ought to be added to the EUI icon set since they don't strike me as generally re-usable (is this for Kibana?).

My recommendation is that these icons be stored elsewhere and referenced via a path or URL as described in the 'Custom SVGs' section at the bottom of this docs page: https://elastic.github.io/eui/#/display/icons

@ryankeairns It's for the new cloud portal we're building. I assume it may become relevant eventually. There are slight difference in the documents and edit icons from the existing document and indexEdit icons. Are there any downsides to adding these? If anything, wouldn't it provide more icon options with EUIIcon.

@cchaos
Copy link
Contributor

cchaos commented Jul 10, 2019

Are there any downsides to adding these? If anything, wouldn't it provide more icon options with EUIIcon.

There are downsides with providing too many icon options. Mostly you'll see people use different icons to mean the same thing and you can lose consistency and recognition for the user. However, I think the documents isn't duplicative (since it's representing multiple) and we can probably just rename this edit one to documentEdit so it's more specific.

The main thing that needs to happen here, though, is that we'll need a designer to pull these svg's into our Sketch library to:

A. Make them consistent with the rest of the glyph set.

Example:

B. Ensure there are no strokes or direct hex values inside the export. This file has both which will cause our theming to not work with these icons.

@claracruz
Copy link
Contributor Author

The main thing that needs to happen here, though, is that we'll need a designer to pull these svg's into our Sketch library to:

A. Make them consistent with the rest of the glyph set.

Example:

B. Ensure there are no strokes or direct hex values inside the export. This file has both which will cause our theming to not work with these icons.

Currently looking into this...

@ryankeairns
Copy link
Contributor

@claracruz The design team is going to discuss our general approach to managing the growing EUI icon library, tomorrow, at our regular design meeting. We'll provide you clearer feedback afterwards, thanks for your patience as we sort out this process.

@claracruz
Copy link
Contributor Author

@claracruz The design team is going to discuss our general approach to managing the growing EUI icon library, tomorrow, at our regular design meeting. We'll provide you clearer feedback afterwards, thanks for your patience as we sort out this process.

ok, no worries

@snide
Copy link
Contributor

snide commented Jul 11, 2019

Summary from the meeting

Follow @cchaos' review above

  • Match the current EUI styles for glyphs. That means round the corners and add the small files
  • Get the SVG files updated to remove fills so they work in dark mode.

I'll add a last one

  • Make sure they are not blurry on non-retina screens. Currently a lot of detail is lost, specifically on the cloud icons there.

image

@claracruz
Copy link
Contributor Author

Summary from the meeting

Follow @cchaos' review above

  • Match the current EUI styles for glyphs. That means round the corners and add the small files
  • Get the SVG files updated to remove fills so they work in dark mode.

I'll add a last one

  • Make sure they are not blurry on non-retina screens. Currently a lot of detail is lost, specifically on the cloud icons there.

image

@cristina-eleonora Are we able to do this pls?

update changelog

Per PR review - Update new icons to icon design specs
@claracruz
Copy link
Contributor Author

claracruz commented Jul 12, 2019

Summary from the meeting
Follow @cchaos' review above

  • Match the current EUI styles for glyphs. That means round the corners and add the small files
  • Get the SVG files updated to remove fills so they work in dark mode.

I'll add a last one

  • Make sure they are not blurry on non-retina screens. Currently a lot of detail is lost, specifically on the cloud icons there.

image

@cristina-eleonora Are we able to do this pls?

Updated PR with new icon designs provided by @cristina-eleonora

@ryankeairns
Copy link
Contributor

I'm in the process of reviewing the SVGs and adding them to our Sketch library. Will report back soon!

@ryankeairns
Copy link
Contributor

ryankeairns commented Jul 12, 2019

@cristina-eleonora There is some potential duplication within the new icons and we're wondering if the existing document and copy icons can be used (A). If not, then we could add both your new document icons (B), but it may lead to a mixed use of the similar document icons. A third alternative would be to add lines to our existing document icon and add your new documents icon (C).

Screenshot 2019-07-12 11 01 41

@claracruz
Copy link
Contributor Author

A third alternative would be to add lines to our existing document icon and add your new documents icon (C).

I second that emotion 👍

@ryankeairns
Copy link
Contributor

I've also touched up the other icons in this PR as well to fit more closely to the EUI style and with non-retina simplifications. Once we settle on the document icon set, I'll either update this PR or open a new one (for simplicity's sake) as I'd also like to change some of the names, etc.

@cristina-eleonora
Copy link

Thank you for your work on this, @ryankeairns

A third alternative would be to add lines to our existing document icon and add your new documents icon (C).

I second that emotion 👍

I third that idea 🙂 I think it can work great with what we need.

@claracruz
Copy link
Contributor Author

I've also touched up the other icons in this PR as well to fit more closely to the EUI style and with non-retina simplifications. Once we settle on the document icon set, I'll either update this PR or open a new one (for simplicity's sake) as I'd also like to change some of the names, etc.

👍

@ryankeairns
Copy link
Contributor

@claracruz @cristina-eleonora I've opened a PR against this one. Presuming everything looks good, you can simply merge the PR: claracruz#1

@ryankeairns
Copy link
Contributor

ryankeairns commented Jul 16, 2019

Here are the pixel-level details. After checking these on a non-retina macbook, I've opted to simplify things a bit here and there, most notably removing the raindrops from the cloudStormy glyph as things got a bit blurred and jumbled at 16x16.

Screenshot 2019-07-16 10 10 53

UPDATE: fixed stormy cloud shape

Screenshot 2019-07-16 11 29 56

@cristina-eleonora
Copy link

LGTM 🙂

@ryankeairns ryankeairns merged commit 364b14b into elastic:master Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants